feat(broker): add durable existing-session delivery - #1851

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

sequenceDiagram
  participant Client
  participant ListenApi
  participant Runtime
  participant WorkerRegistry
  participant NativeDelivery
  participant WorkerSidecar
  Client->>ListenApi: Submit authenticated delivery
  ListenApi->>Runtime: Forward typed delivery request
  Runtime->>WorkerRegistry: Authorize native session
  WorkerRegistry-->>Runtime: Return generation-bound sender
  Runtime->>NativeDelivery: Reserve receipt and deliver
  NativeDelivery->>WorkerSidecar: Write delivery to worker
  WorkerSidecar-->>WorkerRegistry: Report delivery custody outcome
  WorkerRegistry-->>NativeDelivery: Resolve or fail custody waiter
  NativeDelivery-->>Runtime: Return receipt outcome
  Runtime-->>ListenApi: Return JSON response
  ListenApi-->>Client: Return HTTP response

Merge Risk: ⚪ Minimal · up to 18a67

The identified delivery-reporting and worker-release risks are addressed at this head. No actionable code risk remains from this review; merge approval and repository checks remain separate requirements.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 18a67

Live-session checks and duplicate-write protections limit the new delivery path. Persistent receipts also create a resource-exhaustion risk if a caller can submit many distinct deliveries. The intended recovery tradeoff and deployment controls warrant review.

Retained concerns

• Medium · security · inferred: Distinct delivery IDs create durable broker receipts without a fixed retention limit. Repeated authorized delivery can grow persistent broker state and eventually impair delivery or other broker operations unless deployment-level storage controls bound it.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

_{A rabbit checks the queue at dawn
And files each note before it’s drawn
The worker sends a signal clear
A saved receipt records it here
Deferred notes return in line
Then hop to finish, one at a time.}

_{Comment @coderabbitai help to get the list of available commands.}

@codex review

@codex review

To use Codex here, create a Codex account and connect to github.

@coderabbitai review

@coderabbitai review

Exact-head review disposition — bb19d50f1bcc95f1727f6eed6ffb51227ab0d056

I re-audited the current source against every substantive historical finding from Devin, Codex, CodeRabbit, Cursor, Cubic, and GHAS. No finding is being waived:

• Deferred/restart durability and terminal protocol semantics: fixed by durable per-entry queue state, nonterminal delivery_queued, terminal ACK/failure only after final outcome, restart restoration, and immediate deferred failure propagation.
• In-doubt/idempotency propagation: exact duplicates preserve receipt state; in-doubt never becomes duplicate success or causes a second worker write. Durable compact accepted/in-doubt tombstones survive restart and in-memory receipt eviction.
• Receipt retention/capacity and event-loop blocking: the fixed-size whole-ledger design was replaced by indexed per-delivery broker receipts and per-entry sidecar state; filesystem work is off the broker event loop. Broker receipts intentionally persist with broker state because no safe retry horizon exists; per-entry storage removes the fixed count cliff while ordinary authenticated-volume/disk capacity remains operationally managed.
• Durable publication boundaries: reservation and state transitions fsync the file plus required Unix directories/parents. Post-publication sync errors remain committed/in-doubt. Broker queued state requires correlated, generation-scoped sidecar custody rather than stdin flush.
• Worker liveness/custody: authorization rejects dead/unready/wrong-session workers; custody waiters fail on writer, protocol, and all reaper exit paths; replacement generations cannot satisfy stale waiters.
• Dropped reply reconciliation: the runtime response path checks the exact durable receipt off-loop and distinguishes pre-reservation retryable failures from committed/in-doubt outcomes.
• Native command/API validation: inline CLI commands are parsed before provider validation; persistent-only capability advertisement, exact session/lineage/head/protocol/active-input checks, and reconcile tuple checks remain fail-closed.
• Secure queue persistence: private temporary directories and exclusive file creation address the GHAS insecure-temp finding; cleanup after an atomic publish is best effort and cannot reverse success.
• Release/retirement: teardown and session.released run even when persistence fails; normal shutdown_worker retires durable session state and always emits worker_exited (code 0 success, code 1 before propagating cleanup failure). Explicit retirement is the cancellation boundary for still-deferred work; broker queued is historical committed-custody proof, not a current-session liveness assertion, and the delivery ID remains non-replayable.
• Transient persistence liveness: a failed pre-publication in-doubt transition retains the authoritative queued item, emits no false terminal failure, and retries with bounded backoff or duplicate-triggered drain; retry timers reset on progress and cancel on release.
• Post-accept persistence: accepted-tombstone failure never emits a false failure; the already-durable in-doubt tombstone suppresses replay.
• CI regressions: persistent-broker fleet capability expectations include both new actions, and shard-0 lifecycle cleanup was hardened without weakening assertions.
• Documentation: CHANGELOG and PR text scope the contract to persistent brokers and describe idempotent—not stronger “exactly once”—delivery.

Exact-head local evidence: git diff --check; Rust fmt/check/lib-clippy; native receipt/delivery 9/9; generation custody 2/2; harness build/typecheck; focused harness 19/19; full harness 67/67; Prettier. The independent exact-head review is APPROVED and GitHub currently reports zero unresolved threads. Cursor’s exact-head check is green. CodeRabbit’s requested exact-head pass and the remaining CI jobs are still running, so merge remains blocked until every standing gate is terminal.

Resolved both substantive custody-lifecycle findings from CodeRabbit review 5314129342 in exact head d0007a36b20d348296659282bd8cb9041dbec51b.

• WorkerRegistry::release now fails the removed worker generation's native-delivery custody waiters before removing the handle, matching exit/reap fail-closed behavior. A focused regression registers a waiter, releases the worker, and verifies the immediate generation-scoped failure instead of the 10-second timeout.
• WorkerDeliverySender::deliver now owns a drop guard from successful custody registration through custody resolution. Cancellation, write failure, or timeout cancels the exact key; the guard is disarmed only after the custody receiver completes. A focused regression cancels the delivery future after stdin write completion and verifies an exact same-key retry can immediately register.

Fresh local validation on this head:

• cargo test -p agent-relay-broker cancelled_native_delivery_removes_its_custody_waiter --lib — 1/1 pass
• cargo test -p agent-relay-broker release_fails_native_delivery_custody_waiters_before_removal --lib — 1/1 pass
• cargo test -p agent-relay-broker native_delivery_custody --lib — 2/2 pass
• cargo fmt --all -- --check — pass
• cargo check -p agent-relay-broker — pass
• cargo clippy -p agent-relay-broker --lib -- -D warnings — pass
• git diff --check — pass

The prior bb19 approval/GO is superseded. Please review this exact head; merge remains blocked on fresh exact-head approval, terminal-green required CI, CLEAN merge state, and zero substantive unresolved feedback.

@coderabbitai review

Exact-head verdict update: the prior APPROVED review applied only to bb19d50 and has been automatically DISMISSED because the PR head changed to d0007a3. No approval/GO is currently asserted for d0007a3; fresh exact-head review and CI validation are required.

Exact-head bounded clippy proof for d0007a36b20d348296659282bd8cb9041dbec51b:

$ gtimeout 120s cargo clippy -p agent-relay-broker --lib -- -D warnings
    Finished `dev` profile [unoptimized + debuginfo] target(s) in 0.34s
exit code: 0

No timeout or warning suppression was used.

Independent exact-head amendment verdict for d0007a36b20d348296659282bd8cb9041dbec51b: NO-GO / approval withheld.

The two changes in bb19d50..d0007a36 correctly close the generation-scoped release-waiter and cancelled-delivery waiter leaks, and the focused custody tests plus fmt/check/lib-clippy/diff checks pass. The retained full-review items have the following dispositions:

1. Valid runtime blocker — restored deferred final outcome. runAiSdkSidecar registers its event listener and calls restoreDeferredMessages() before stdin can rebuild relayDeliveries. A restored entry can therefore emit delivery.accepted or delivery.failed with no volatile delivery mapping, and exact head returns without writing the terminal delivery_ack / delivery_failed frame. Fix and regression coverage for both terminal outcomes are required.
2. Contract clarification required — lineage/head. Exact-head live authorization is correctly anchored to the broker-owned worker name, session id, generation, native protocol/readiness, and active-input capability. lineageId and headSha are caller assertions used for immutable receipt/reconciliation matching, not independently attested worker claims. The code/API contract and PR body must state that accurately and cite the upstream authority/binding; the current body’s claim that wrong lineage/head is rejected as live-worker authorization is misleading.
3. Valid process gate — PR description. The body does not currently include the repository template’s ## Test Plan checklist or ## RelayFlow Proof fields. This user-visible feature requires feature/bugfix plus exactly one real tests/relayflows/cases/<case-id>/ case.
4. Advisory, not independently substantiated as repository policy — 36.04% docstring metric. I found no checked-in 80% docstring rule in AGENTS.md, repository workflows, or configuration. This is not by itself a substantive merge blocker, but it needs an explicit public disposition; security-sensitive contract fields should still be documented.

The reported sf-mini transcript is not accepted as evidence because its save failed under ENOSPC. The owner identified the volume exhaustion and removed only disposable build cache; remote exact-head CI and independently reproducible commands remain the gate.

No approval/GO applies to d0007a36. Any fix changes the head and requires a fresh full exact-head audit. Merge remains gated on terminal-green required CI, unchanged reviewed head, CLEAN merge state, and zero substantive unresolved feedback.

CodeRabbit retained-summary disposition — exact head 26047bb97712f1844a06dbe1c6c070b01230616b

All four retained items were re-audited; merge remains blocked pending fresh exact-head CI and approval.

1. Restored deferred final outcome — valid, fixed. The sidecar no longer requires the volatile relayDeliveries map to report a restored terminal outcome. delivery.accepted and delivery.failed now fall back to the durable event's messageId for event_id, while still preferring the live map for an in-process delivery. The regression restarts the sidecar with two durably queued entries and proves both the restored delivery_ack and restored delivery_failed frames retain their original event IDs.
2. lineageId / headSha binding — contract clarification, not a worker-authorization control. The live target is authorized from broker-owned state: worker name, process generation/liveness/readiness, exact native session ID, Codex/native protocol, and activeInput. Lineage/head are authenticated-caller assertions used only to bind the immutable receipt digest and exact reconciliation tuple. They cannot select or widen the target session. The Rust request fields now document that boundary and the PR body no longer claims lineage/head are independently checked against the worker. Callers remain responsible for binding those assertions to their trusted live-session record before invoking this control-plane capability.
3. Description/template proof — valid, fixed. The PR body now contains the required checked Test Plan and declares change type feature plus exactly one new case, tests/relayflows/cases/1851-native-existing-session-delivery/. Its base/head proof executes the exact supplied Linux broker binary: base must omit both actions; head must advertise both versioned action contracts and durable receipt/reconciliation metadata. The manifest passes the repository's validateCaseManifest.
4. Docstring 36.04% — advisory, not a repository gate. There is no repository policy or compiler lint requiring 80% coverage, and the heuristic counts private/test helpers across every touched file. Mass comments on unrelated helpers would add noise rather than contract clarity. The newly clarified request/reconciliation fields now have focused Rust documentation; existing public behavior remains covered by module/type/function docs and tests.

Fresh local evidence:

• focused sidecar: 1 file / 10 tests passed;
• full harness: 11 files / 68 tests passed;
• harness TypeScript check passed;
• RelayFlow manifest validation and runner syntax passed;
• Prettier, cargo fmt --check, and git diff --check passed.

Disk-evidence note: before these edits, /System/Volumes/Data had 115 MiB free and was at 100%. cargo clean removed only this worktree's disposable Rust build cache (10,859 files; 7.1 GiB reported), restoring 5.2 GiB free. No repository, worktree source, or user data was removed. Remote exact-head CI remains authoritative.

Independent exact-head update for 26047bb97712f1844a06dbe1c6c070b01230616b: NO-GO. The new CodeRabbit actionable is valid: tests/relayflows/cases/1851-native-existing-session-delivery/run.mjs supplies RELAY_NODE_TOKEN=nt_fixture_native_delivery_proof, but the imported engineFixture requires exactly Bearer nt_fixture_task_proof. The WebSocket upgrade therefore cannot succeed and the proof cannot observe node.register; manifest validation and syntax checking do not exercise this. Fix or parameterize the token, execute both exact base/head proof arms, resolve the thread, and re-run fresh-head reviews/CI. No approval applies to this SHA.

Independent exact-head gate status for 240bf041b065bb705f36ae469a36a37732763b1a: HOLD — no GO/approval yet.

The technical audit is clean:

• restored deferred delivery_ack and delivery_failed reporting is fixed and regression-covered;
• lineageId/headSha are accurately documented as authenticated-caller receipt assertions, not live-worker authorization claims;
• required Test Plan and RelayFlow Proof are present;
• the proof-runner credential mismatch from 26047bb is fixed;
• the executable Linux targeted verification containing corpus-1851-native-existing-session-delivery passed in shard 5;
• CodeRabbit and Cursor are clean; unresolved review threads: 0;
• exact-head remote rollup is terminal with 74 successes, one neutral, one skipped, and zero failures. The sole cancelled Build exact Linux broker run (36105530908) was concurrency-superseded on the same SHA by successful run 36105565316.
• independent local evidence: focused sidecar 10/10; full harness 68/68 after an isolated cold-load timeout passed on immediate focused and full reruns; harness TypeScript check; manifest validation; runner syntax; Prettier, Rust fmt, and diff checks all pass.

GitHub still reports mergeStateStatus=BLOCKED and reviewDecision=REVIEW_REQUIRED, not CLEAN. The available kjgbot credential authored the 26047bb/240bf041 fix commits, so using it for a formal approval would not be independent. Under the explicit gate amendment, I will not label this SHA GO until a genuinely independent non-author maintainer records the required approval and the unchanged head recalculates to CLEAN with zero new findings. Do not merge yet.

Independent exact-head audit — 18a67dae216d6f20b3d8ab05475e8e59a814b4bd (relay-1851-driver-0925d)

Verdict: technical GO, no open defects found. A separate maintainer approval is still required. kjgbot pushed 18a67dae, so this comment is evidence, not an approval.

Audit of 240bf041: 2 defects found, both fixed in 18a67dae

1. Custody ID collision. relay_delivery() put the caller's raw deliveryId on the worker wire. worker_events.rs resolves native custody on any delivery_queued/delivery_ack whose delivery_id matches, and ordinary delivery IDs are chosen externally (fleet.rs deliver.delivery_id, del_*). A colliding ordinary ACK could therefore mark a native receipt queued without sidecar custody. Fix: the wire delivery_id is now the receipt ID ndr_<sha256(deliveryId)>, and the caller key is carried as event_id. The sidecar treats delivery_id as opaque (sidecar.ts echoes it), so restart dedupe is unaffected.
2. Receipt I/O on the event loop, contradicting the PR body. existing_outcome/reconcile_receipt ran synchronously in the runtime and fleet actors, and create_receipt/save_receipt (with fsyncs) ran directly on tokio worker threads. Fix: all receipt I/O goes through spawn_blocking. The actors do only in-memory authorization and then spawn deliver_authorized, which checks the exact duplicate, then authorization, then reserve and deliver. An exact duplicate is still answered from its receipt after the worker exits (new regression test). A join failure after reservation maps to in_doubt.

Verified to hold (with file evidence)

• Atomic reservation: persist_noclobber after a file fsync, then a directory and parent fsync. A sync failure after publication is reported as in_doubt.
• A changed tuple conflicts. An exact duplicate never causes a second write. An in_doubt receipt never reports duplicate success.
• Authorization comes before reservation: the worker must be live, ready, Codex, the exact generation and session, native protocol v1, and have activeInput. lineageId/headSha never reach authorization, and the docstrings say so accurately.
• Custody waiters are generation-scoped and fail on writer failure, exit, release and reaper paths; the drop guard cancels on timeout or cancellation.
• Ephemeral brokers neither advertise nor accept the actions, and the HTTP routes require auth.
• Filenames are sha256 hashes, so path traversal isn't possible. Identifiers are capped at 512 bytes and messages at 128 KiB.
• The restart-outcome fix (26047bb) is correct: the in_doubt tombstone is persisted before handoff to the host, restored delivery_ack/delivery_failed fall back to the durable messageId, and tombstoned entries are never replayed.
• RelayFlow case 1851 uses bearer nt_fixture_task_proof, which matches engine-fixture.mjs, and its case.json matches the sibling cases.
• CHANGELOG is under [Unreleased - Major] → Added.

A fresh reviewer re-audited the 240bf041..18a67dae delta and found no regressions. The Fleet error and output payload formats are unchanged.

Local evidence (macOS, worktree at 18a67dae)

$ cargo test -p agent-relay-broker native_delivery --lib
test native_delivery::tests::worker_wire_delivery_id_is_namespaced_by_the_receipt ... ok
test runtime::tests::native_delivery_runtime_answers_exact_duplicate_after_worker_exit ... ok
test result: ok. 15 passed; 0 failed; 0 ignored; 0 measured; 1317 filtered out
$ cargo fmt --all -- --check        -> exit 0
$ cargo clippy -p agent-relay-broker --lib -- -D warnings   -> exit 0
$ cargo test -p agent-relay-broker --lib
test result: FAILED. 1321 passed; 6 failed

The 6 local failures are unrelated to this change:

• 5 are spawner::tests::broker_hook_*, which fail identically on the unmodified 240bf041 (stash baseline: 5 failed) because of local git-hook environment state.
• 1 is cli_mcp_args::tests::codex_output_matches_authority_function, an env-var-race flake in an untouched module (1 failure in 3 isolated runs). All of these pass in CI.

CI at 18a67dae

gh pr checks 1851 → 75 pass, 2 skipping (Swift SDK Tests, cubic), 0 fail/pending. The first Coverage (upload) attempt failed on an unrelated 5s timeout in packages/cli/src/cli/lib/attach-fleet-node.test.ts (a CLI package this PR doesn't touch). A re-run of the failed job (run 36222673955) passed. The earlier cancelled Build exact Linux broker run (36105530908, on 240bf041) was superseded by cancel-in-progress concurrency on the same SHA; run 36105565316 passed.

Low-severity notes (not blocking)

• ndr_ IDs are deterministic, so a trusted engine could still forge one deliberately. The fix removes accidental collisions only. Optional hardening: reject ndr_-prefixed ordinary delivery IDs.
• deliver_authorized and reserve_and_deliver each do a blocking duplicate lookup, so there's one redundant small read per new delivery.
• Native Fleet replies bypass the sending fleet action result info log.
• Sidecar release() deletes still-queued deferred entries and their tombstones without emitting delivery_failed. This matches the documented "explicit session retirement is the cancellation boundary". The broker receipt still prevents native replay.
• The restore path throws on a corrupt queue file (it fails closed) instead of quarantining it.
• The RelayFlow case exercises capability advertisement only. The durable restart paths are covered by the harness unit tests.

Unresolved review threads: 0. Merge still requires a fresh independent maintainer approval of this exact head.

Independent exact-head audit 45b5071 — GO

Account: khaliqgant. Because this is the PR author account, this is a comment rather than an APPROVE review.

I independently audited the full PR diff and the exact head 45b5071.

Evidence:

• Native receipt reservation is atomic and durable before the sole worker write: a synced temporary receipt is published with no-clobber semantics, directory sync failures after publication become in_doubt, and the queued transition happens only after correlated sidecar custody.
• Exact duplicates are bound to the complete request digest and return the durable queued receipt without another write; changed input conflicts. An in_doubt receipt returns native_delivery_in_doubt with committed=true and is never returned as queued or duplicate success.
• New deliveries authorize before reservation against a live, ready Codex worker with the exact session ID, native protocol v1, and activeInput capability. Custody is keyed by worker name, process generation, and a broker-namespaced ndr delivery ID; stale-generation events are rejected, and release, exit, writer failure, cancellation, and timeout paths fail closed.
• Both HTTP routes are inside the broker API-key middleware. Production startup always configures or generates that key. Ephemeral brokers reject the durable operation and do not advertise the two native delivery actions; only persistent brokers append those manifest capabilities.
• I audited the merge delta 18a67da..45b5071. The fix(broker): scope credentials passed to spawned workers #1854 credential scrub does not break the native AI SDK sidecar: inherited Relay credentials are removed first, then the explicit worker_env restores delegated RELAY_WORKSPACE_KEY and RELAY_API_KEY, and the direct native sidecar receives its own RELAY_AGENT_TOKEN. native-relay-tools also falls back from RELAY_WORKSPACE_KEY to RELAY_API_KEY.
• git diff --check is clean. gh pr checks at this exact head is terminal and green across the current check set. The older duplicate Build exact Linux broker job was cancelled and its exact-head rerun completed successfully.

No concrete correctness or security defect was found. GO applies only to exact head 45b5071.